Skip to content

feat(agent): wire Conjur JWT config into the top-level CyberArk client - #823

Open
roeezis wants to merge 6 commits into
jetstack:masterfrom
roeezis:split/06-client-wiring
Open

feat(agent): wire Conjur JWT config into the top-level CyberArk client#823
roeezis wants to merge 6 commits into
jetstack:masterfrom
roeezis:split/06-client-wiring

Conversation

@roeezis

@roeezis roeezis commented Aug 23, 2026

Copy link
Copy Markdown

Summary

Part 6 of the SMS/Conjur JWT authentication series (split out of #817). Stacked on the prior 5 PRs in this series — diff will shrink as they merge.

NewCyberArk and the agent config schema were still legacy-username/password-only at the top level, even though the underlying authenticator selection (#822) already supports Conjur JWT. Threads service_id/account/jwt_source/jwt_file_path through cyberark.service_id in the agent config down to NewCyberArk, so the config file is the single place an operator chooses which authentication method to use.

config.go's call site and client_cyberark.go's signature change together — splitting them across two PRs would leave one non-building at every commit in between, so they're one PR.

Test plan

  • go build ./...
  • go test ./pkg/agent/... ./pkg/client/... ./pkg/testutil/...

rzisholz added 4 commits August 24, 2026 14:43
Introduces a small, isolated interface for reading a JWT from a file
path — the first piece of the upcoming Conjur JWT authentication path,
split out on its own since nothing else in this PR depends on it yet.
identity.go mixed the shared client/token-cache plumbing with the
CyberArk Identity username/password (UP) login flow. Move the UP-specific
code into username_password.go so the shared plumbing stays easy to find
once a second login mechanism (Conjur JWT) is added alongside it.

No behavior change — pure extraction, plus exporting the mock's success
credentials for other packages' tests.
The Service Discovery API returns several independently-hosted services;
the authn-jwt exchange this PR series adds is served by secrets_manager,
a different host from identity_administration. Add a SecretsManager
field to Services and parse it, so callers have it available — nothing
reads it yet, that lands in a later PR alongside the client that needs
it.

secrets_manager and discoveryContext are deliberately not required here
unlike identity, since not every caller needs them and requiring
secrets_manager would break every existing username/password install on
a tenant not yet onboarded to Conjur — each caller validates what it
needs at its own point of use instead.

Factor the repeated "find the first active main endpoint" loop into a
mainActiveAPI helper now that there are three near-identical copies.
Exchanges a projected ServiceAccount JWT (via jwtsource) for a Conjur
access token through the authn-jwt endpoint, and authenticates requests
with it as identity.RequestAuthenticator. Nothing wires this in yet —
that's the next PR, once both this and the legacy identity client exist
side by side.

The identity returned for audit tagging is the token's own sub claim
when it can be extracted, falling back to the configured service ID
otherwise. The cache expiry is driven by the token's own exp claim when
present, falling back to a guessed TTL only when it isn't — a fixed TTL
stamped after the exchange returns would otherwise serve a token past
its real expiry under latency or clock skew. Exposes Invalidate() so a
caller that gets a 401 from the resource server can force a fresh
exchange instead of waiting out the cache.
@roeezis
roeezis force-pushed the split/06-client-wiring branch from c2d0dcf to 9b587c8 Compare August 24, 2026 11:45

@mladen-rusev-cyberark mladen-rusev-cyberark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config plumbing is straightforward, and the reasoning for combining the call-site and signature change in one commit is right. Two things ride along that I'd separate, or at least call out — inline.

Minor points:

  • NewCyberArk now takes four adjacent same-typed string params (pkg/client/client_cyberark.go:45) — classic transposition hazard, and the compiler can't help. CyberArkConfig already exists and is already being destructured at the call site; passing the struct would be one self-documenting parameter, and adding a fifth field later wouldn't be a signature change.
  • pkg/agent/config.go:783: after the block above it, clusterName == "" already implies cfg.ClusterID == "", so the second conjunct is dead.
  • res.CyberArk = ark only runs in MachineHub mode, so a cyberark: block in a Venafi Cloud config is silently ignored. Everywhere else in this function ignored fields get a log line (see the organization_id handling just above).

Comment thread pkg/agent/config.go
}
if clusterName == "" && cfg.ClusterID != "" {
log.Info("Using cluster_id as cluster_name for backwards compatibility", "clusterID", cfg.ClusterID)
clusterName = cfg.ClusterID

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the MachineHub cluster-name fallback from ARK_USERNAME to cluster_id, and promotes cluster_id from explicitly-ignored to load-bearing. The change is motivated — ARK_USERNAME doesn't exist on the JWT path — but an existing install relying on the old fallback that also sets cluster_id will start reporting under a different cluster name after upgrade, possibly fragmenting its history in Discovery & Context. Installs with neither now report an empty cluster name.

Three asks:

  1. Mention it in the commit message and PR description — as written both are entirely about Conjur config threading, so a reader wouldn't know cluster naming moved.
  2. Confirm with whoever owns the MachineHub data model that a cluster-name change on upgrade is acceptable, and whether a migration note is needed.
  3. The log line says "for backwards compatibility", but using cluster_id is the new behaviour — worth rewording.

Given it's independently reviewable and independently riskier than the rest of this PR, it may deserve its own.

Comment thread pkg/testutil/envtest.go
Comment thread pkg/agent/config.go
Adds NewRequestAuthenticator, choosing between the new Conjur JWT
exchange and the legacy CyberArk Identity username/password login based
on which config is present — Conjur JWT takes priority when both are
set. Conjur JWT requires service_id and resolves its base URL from the
secrets_manager service discovered in the prior PR, not
identity_administration; the two are different hosts. The identity API
is only required on the username/password path now; Conjur JWT never
uses it.

Switches keyfetch's client over to the new authenticator selection
instead of constructing a username/password identity client directly.
Doing so dropped keyfetch's own per-fetch LoginUsernamePassword call, which
was the only thing keeping the cached identity token from aging out —
identity.Client.AuthenticateRequest never refreshed on its own. Give it
the same self-refreshing behavior conjur.Client already has: it now
re-logs-in internally once its cached token passes tokenTTL (a field,
not a const, so tests can shrink it), using a durable copy of the
credentials it captured at the last LoginUsernamePassword call — the
caller's own password slice is still zeroed as before. A refresh that
fails falls back to whatever's cached rather than failing the request
outright, since the next call will retry.

Hoists the jwt_source validation and the "file"/"conjur" literals this
and the agent config layer both encode separately into shared
JWTSourceFile/DefaultAccount consts and a ValidateJWTSource helper, and
drops the "POC" wording from the operator-facing error.
@roeezis
roeezis force-pushed the split/06-client-wiring branch 2 times, most recently from c304519 to a696848 Compare August 24, 2026 12:16
@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Fixed — added FakeCyberArkUsernamePassword and a matching integration test (TestCyberArkClient_PostDataReadingsWithOptions_UsernamePasswordMockAPI) that leaves serviceID empty, so the legacy path is what's actually under test rather than silently exercising Conjur.

On the signature note: kept FakeCyberArk's existing shape as-is (minimal blast radius) and gave the new function its own return shape (httpClient, username, password) instead — two constructors, each specific to its auth method, rather than forcing one shared shape.

@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Fixed — added the check you suggested (same code, os.Getenv("ARK_USERNAME")), and reworded the comment to describe what's actually checked and why (deferring to selectAuthenticator alone would leave a misconfigured agent reporting healthy for up to a full config.period). Updated the two existing tests that asserted "valid at config time" to set ARK_USERNAME (preserving what they were actually testing — the username/password fallback still works without service_id), and added a negative test for neither being set.

@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Confirmed accurate, and I'm not resolving this one — it's a product decision, not something I should silently pick:

  • Old behavior: cluster_id was explicitly ignored in MachineHub mode; the fallback was ARK_USERNAME.
  • New behavior (this PR): falls back to cluster_id, which is now load-bearing.

An existing install with ARK_USERNAME set AND a cluster_id in config (previously just ignored) will report under a different cluster name post-upgrade, and one with neither set will report an empty name where it previously had ARK_USERNAME's value.

I've reworded the log line (point 3 — it said "for backwards compatibility" describing what is actually new behavior). Points 1 and 2 need a human call: @rzisholz — is a cluster-name change on upgrade acceptable here, and does whoever owns the MachineHub data model need to weigh in or get a migration note? I don't have the context to decide this myself, and I don't want to guess and silently ship a behavior change to production cluster identity.

NewCyberArk and the agent config schema were still legacy-username/password-only
at the top level, even though the underlying authenticator selection (previous
PR) already supports Conjur JWT. Threads service_id/account/jwt_source/jwt_file_path
through cyberark.service_id in the agent config down to NewCyberArk, so the
config file is the single place an operator chooses which authentication method
to use.

config.go's call site and client_cyberark.go's signature change together —
splitting them across two PRs would leave one non-building at every commit
in between.

jwt_source validation now delegates to the shared cyberark.ValidateJWTSource
(previous PR) instead of re-implementing the same rule with different wording.

Requiring an auth method (service_id or ARK_USERNAME) is now checked at
config-validation time rather than left to cyberark.selectAuthenticator alone
— that only runs at first upload, so a misconfigured agent could otherwise
report healthy for up to a full config.period before failing.

Adds FakeCyberArkUsernamePassword and a matching integration test: the only
existing test exercising NewCyberArk's username/password path set
ARK_USERNAME/ARK_SECRET but also passed a non-empty serviceID, which
selectAuthenticator prioritises — so it silently tested Conjur regardless of
those env vars, leaving the legacy path with no coverage through this seam.
@roeezis
roeezis force-pushed the split/06-client-wiring branch from a696848 to 8aa61e4 Compare August 24, 2026 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants